fix: allow third-party backup provider plugins via configurable allowed list#13332
fix: allow third-party backup provider plugins via configurable allowed list#13332yigitbasalma wants to merge 3 commits into
Conversation
|
Congratulations on your first Pull Request and welcome to the Apache CloudStack community! If you have any issues or are unsure about any anything please check our Contribution Guide (https://github.com/apache/cloudstack/blob/main/CONTRIBUTING.md)
|
|
@blueorangutan package |
|
@DaanHoogland a [SL] Jenkins job has been kicked to build packages. It will be bundled with no SystemVM templates. I'll keep you posted as I make progress. |
There was a problem hiding this comment.
Pull request overview
This PR makes the backup provider plugin selection extensible by replacing the hardcoded built-in plugin allowlist with a new configuration key so administrators can permit third-party backup provider plugins without modifying CloudStack source.
Changes:
- Adds
backup.framework.provider.plugin.allowed(comma-separated) to define which backup provider plugins are permitted. - Updates validation for
backup.framework.provider.pluginto check against the configured allowlist instead of a hardcoded list. - Updates the
backup.framework.provider.pluginconfig description to reference the new allowlist setting.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| ConfigKey<String> AllowedBackupProviderPlugins = new ConfigKey<>( | ||
| "Advanced", String.class, | ||
| "backup.framework.provider.plugin.allowed", | ||
| "dummy,veeam,networker,nas", | ||
| "Comma-separated list of allowed backup provider plugins.", | ||
| true, ConfigKey.Scope.Zone); |
| @@ -254,9 +262,14 @@ static void validateBackupProviderConfig(String value) { | |||
| if (value != null && (value.contains(",") || value.trim().contains(" "))) { | |||
| throw new IllegalArgumentException("Multiple backup provider plugins are not supported. Please provide a single plugin value."); | |||
| } | |||
| List<String> validPlugins = List.of("dummy", "veeam", "networker", "nas"); | |||
| if (value != null && !validPlugins.contains(value)) { | |||
| throw new IllegalArgumentException("Invalid backup provider plugin: " + value + ". Valid plugin values are: " + String.join(", ", validPlugins)); | |||
| String allowed = AllowedBackupProviderPlugins.value(); | |||
| if (allowed != null && value != null) { | |||
| List<String> validPlugins = Arrays.asList(allowed.split(",")); | |||
| if (!validPlugins.contains(value.trim())) { | |||
| throw new IllegalArgumentException("Invalid backup provider plugin: " + value + | |||
| ". Valid plugin values are: " + allowed + | |||
| ". You can add more plugins via backup.framework.provider.plugin.allowed setting."); | |||
| } | |||
| } | |||
|
Packaging result [SF]: ✔️ el8 ✔️ el9 ✔️ el10 ✔️ debian ✔️ suse15. SL-JID 18256 |
|
@yigitbasalma , I had a quick look at your code and it looks like it could work, but how about allowing adding a plugin name to the existing config (“backup.framework.provider.plugin”)? It seems to me that would be more user friendly. we need to validate the provider anyway and that way a typo by the admin can occur in only one place. |
|
@DaanHoogland Updated as suggested — removed the allowed list config and hardcoded validation. The provider name set in backup.framework.provider.plugin is now validated at runtime by Spring when loading the plugin bean. |
|
@blueorangutan package |
|
@DaanHoogland a [SL] Jenkins job has been kicked to build packages. It will be bundled with no SystemVM templates. I'll keep you posted as I make progress. |
Codecov Report✅ All modified and coverable lines are covered by tests.
Additional details and impacted files@@ Coverage Diff @@
## main #13332 +/- ##
=============================================
- Coverage 17.67% 3.53% -14.15%
=============================================
Files 5922 464 -5458
Lines 533165 40210 -492955
Branches 65208 7565 -57643
=============================================
- Hits 94242 1421 -92821
+ Misses 428276 38599 -389677
+ Partials 10647 190 -10457
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
|
@DaanHoogland FYI: Workflow build failed due to an unused import error. I've updated the code, verified the fix, and committed the changes to the PR. |
|
Packaging result [SF]: ✖️ el8 ✖️ el9 ✖️ debian ✖️ suse15. SL-JID 18328 |
Description
Currently, the
backup.framework.provider.pluginsetting validates against a hardcoded list of plugins (dummy,veeam,networker,nas). This prevents third-party backup provider plugins from being registered and used in CloudStack, even when the plugin is correctly implemented and deployed.This PR introduces a new configuration key
backup.framework.provider.plugin.allowedthat contains a comma-separated list of allowed backup provider plugins. Administrators can extend this list to include third-party plugins without modifying CloudStack source code.Default value maintains full backward compatibility with existing plugins.
Types of changes
Feature/Enhancement Scale or Bug Severity
Feature/Enhancement Scale
Bug Severity
Screenshots (if appropriate):
N/A
How Has This Been Tested?
backup.framework.provider.plugin.allowedincludes all existing plugins (dummy,veeam,networker,nas)How did you try to break this feature and the system with this change?
dummy,veeam,networker,nas) — all work as before